Adds saml auth header to differentiate saml requests and prevents auto login as anonymous user when basic authentication fails by DarshitChanpura · Pull Request #4152 · opensearch-project/security

DarshitChanpura · 2024-03-21T01:30:56Z

Description

This PR fixes a bug where SAML and Anonymous auth cannot co-exist. At present, enabling SAML along with anonymous auth causes SAML Login to fail before it hits the IdP because both SAML and anonymous auth are wired to not have credentials on the first login attempt, causing the authn check against the auth domains to be skipped (see here). This, eventually, results in setting the default user as anonymous (see here) since anonymous auth is enabled.

This PR also resolves another bug involving anonymous auth where a user would be automatically logged in as anonymous user upon failed basic authentication if anonymous auth is enabled.

Category: Bug fix
Why these changes are required?
- To allow SAML auth to work when anonymous auth is enabled
- To prevent auto login as anonymous user upon failed basic authentication

Security-dashboards companion PR: opensearch-project/security-dashboards-plugin#1839

Issues Resolved

Related [BUG] Having Anonymous and SAML as sign in options does not work. SAML Login fails when Anonymous is enabled security-dashboards-plugin#1731

Testing

Automated

Check List

New functionality includes testing
~~- [ ] New functionality has been documented~~
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

codecov · 2024-03-21T04:38:25Z

Codecov Report

Attention: Patch coverage is 53.84615% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 66.20%. Comparing base (4e8297a) to head (4a6dff7).
Report is 4 commits behind head on main.

❗ Current head 4a6dff7 differs from pull request most recent head 5174654. Consider uploading reports for the commit 5174654 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4152   +/-   ##
=======================================
  Coverage   66.19%   66.20%           
=======================================
  Files         301      301           
  Lines       21709    21712    +3     
  Branches     3505     3505           
=======================================
+ Hits        14371    14375    +4     
+ Misses       5580     5578    -2     
- Partials     1758     1759    +1

Files	Coverage Δ
.../org/opensearch/security/auth/BackendRegistry.java	`61.58% <53.84%> (-0.96%)`	⬇️

... and 5 files with indirect coverage changes

src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

src/main/java/org/opensearch/security/auth/BackendRegistry.java

src/integrationTest/java/org/opensearch/security/http/BasicWithAnonymousAuthTests.java

derek-ho

I don't have any concerns with the implementation of this change, since I think it preserves functionality, but trying to understand more about @peternied's concern with the headers

src/main/java/org/opensearch/security/auth/BackendRegistry.java

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

src/main/java/org/opensearch/security/auth/BackendRegistry.java

stephen-crawford

Looks good to me!

src/integrationTest/java/org/opensearch/security/http/BasicWithAnonymousAuthTests.java

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura · 2024-04-09T01:15:44Z

~~All failures caused by:~~

java.lang.NoSuchMethodError: 'void org.opensearch.transport.TcpTransport.inboundMessage(org.opensearch.transport.TcpChannel, org.opensearch.transport.ProtocolInboundMessage)'

~~Root-cause is likely this upstream change: opensearch-project/OpenSearch#12967.~~

Update: This issue has been resolved.

stephen-crawford

Looks good to me

…o login as anonymous user when basic authentication fails (#4152) Signed-off-by: Darshit Chanpura <dchanp@amazon.com> (cherry picked from commit d87ab3f) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…o login as anonymous user when basic authentication fails (opensearch-project#4152) Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

This was referenced Mar 21, 2024

Skips authentication only for anonymous auth requests when anonymous-auth is enabled #4097

Closed

Fixes saml login flow to work with anonymous auth opensearch-project/security-dashboards-plugin#1839

Merged

Adds saml auth header to differentiate saml requests

7f3c50b

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura force-pushed the saml-anony-bug-fix branch from 13d5c33 to 7f3c50b Compare March 21, 2024 01:45

Prevents auto login as anonymous upon failed authentication

03459a2

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura changed the title ~~Adds saml auth header to differentiate saml requests~~ Adds saml auth header to differentiate saml requests and prevents auto login as anonymous user when basic authentication fails Mar 21, 2024

Fixes an apocalyptic level typo

3aecdc6

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura force-pushed the saml-anony-bug-fix branch from 8af4ef6 to 61a7b35 Compare March 21, 2024 04:17

DarshitChanpura mentioned this pull request Mar 21, 2024

[BUG] Having Anonymous and SAML as sign in options does not work. SAML Login fails when Anonymous is enabled opensearch-project/security-dashboards-plugin#1731

Closed

DarshitChanpura commented Mar 21, 2024

View reviewed changes

src/integrationTest/java/org/opensearch/security/DoNotFailOnForbiddenTests.java Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/main' into saml-anony-bug-fix

e3384d2

DarshitChanpura force-pushed the saml-anony-bug-fix branch from 61a7b35 to e3384d2 Compare March 26, 2024 16:30

DarshitChanpura marked this pull request as ready for review March 26, 2024 17:05

DarshitChanpura requested review from RyanL1997, cliu123, cwperks, davidlago, peternied, reta, stephen-crawford and willyborankin as code owners March 26, 2024 17:05

DarshitChanpura added 3 commits March 26, 2024 13:25

Adds tests to verify the new check

1113b64

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Adds a comment

95e2611

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Fixes a test

87a65e2

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

peternied reviewed Mar 26, 2024

View reviewed changes

src/main/java/org/opensearch/security/auth/BackendRegistry.java Outdated Show resolved Hide resolved

derek-ho reviewed Mar 29, 2024

View reviewed changes

src/integrationTest/java/org/opensearch/security/http/BasicWithAnonymousAuthTests.java Outdated Show resolved Hide resolved

derek-ho reviewed Mar 29, 2024

View reviewed changes

src/integrationTest/java/org/opensearch/security/http/BasicWithAnonymousAuthTests.java Outdated Show resolved Hide resolved

derek-ho reviewed Mar 29, 2024

View reviewed changes

peternied previously approved these changes Mar 29, 2024

View reviewed changes

cwperks reviewed Mar 29, 2024

View reviewed changes

src/main/java/org/opensearch/security/auth/BackendRegistry.java Outdated Show resolved Hide resolved

Fixes typo

244f1c5

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura dismissed peternied’s stale review via 244f1c5 March 29, 2024 19:37

cwperks previously approved these changes Apr 2, 2024

View reviewed changes

peternied suggested changes Apr 2, 2024

View reviewed changes

src/main/java/org/opensearch/security/auth/BackendRegistry.java Outdated Show resolved Hide resolved

DarshitChanpura dismissed cwperks’s stale review via 0a08132 April 2, 2024 17:19

peternied previously approved these changes Apr 3, 2024

View reviewed changes

stephen-crawford previously approved these changes Apr 4, 2024

View reviewed changes

src/integrationTest/java/org/opensearch/security/http/BasicWithAnonymousAuthTests.java Outdated Show resolved Hide resolved

DarshitChanpura added 3 commits April 8, 2024 13:19

Renames url parameter

a8f25d8

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Fixes integ tests

19a597c

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Addresses PR comment and consumes auth_type param for authinfo request

b54b619

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura dismissed stale reviews from stephen-crawford and peternied via b54b619 April 8, 2024 18:51

DarshitChanpura force-pushed the saml-anony-bug-fix branch from 4a6dff7 to b54b619 Compare April 8, 2024 18:51

Merge remote-tracking branch 'upstream/main' into saml-anony-bug-fix

5174654

stephen-crawford approved these changes Apr 9, 2024

View reviewed changes

cwperks approved these changes Apr 9, 2024

View reviewed changes

cwperks added the backport 2.x label Apr 9, 2024

DarshitChanpura merged commit d87ab3f into opensearch-project:main Apr 9, 2024

opensearch-trigger-bot bot mentioned this pull request Apr 9, 2024

[Backport 2.x] Adds saml auth header to differentiate saml requests and prevents auto login as anonymous user when basic authentication fails #4228

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds saml auth header to differentiate saml requests and prevents auto login as anonymous user when basic authentication fails#4152

Adds saml auth header to differentiate saml requests and prevents auto login as anonymous user when basic authentication fails#4152
DarshitChanpura merged 12 commits intoopensearch-project:mainfrom
DarshitChanpura:saml-anony-bug-fix

DarshitChanpura commented Mar 21, 2024 •

edited

Loading

Uh oh!

codecov bot commented Mar 21, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

derek-ho left a comment

Uh oh!

Uh oh!

Uh oh!

stephen-crawford left a comment

Uh oh!

Uh oh!

DarshitChanpura commented Apr 9, 2024 •

edited

Loading

Uh oh!

stephen-crawford left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

DarshitChanpura commented Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Testing

Check List

Uh oh!

codecov bot commented Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

derek-ho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stephen-crawford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DarshitChanpura commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephen-crawford left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DarshitChanpura commented Mar 21, 2024 •

edited

Loading

codecov bot commented Mar 21, 2024 •

edited

Loading

DarshitChanpura commented Apr 9, 2024 •

edited

Loading